-
Notifications
You must be signed in to change notification settings - Fork 879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Assigning Address driver options #707
Conversation
2a932a4
to
5d2db71
Compare
Can someone poke CI? It timed out |
@@ -690,7 +690,7 @@ func (ep *endpoint) DataScope() string { | |||
return ep.getNetwork().DataScope() | |||
} | |||
|
|||
func (ep *endpoint) assignAddress() error { | |||
func (ep *endpoint) assignAddress(adOptions map[string]string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to have an EndpointCreateOptionIpam()
setter for CreateEndpioint()
logic which would set a preferred address and a map of options for this endpoint. Similarly to NetworkOptionIpam()
for network create.
Then this function could directly access it as ipamAllocateVersion()
does for network.
@aboch How does that look? |
527e83a
to
e3c3479
Compare
@@ -637,6 +639,14 @@ func EndpointOptionGeneric(generic map[string]interface{}) EndpointOption { | |||
} | |||
} | |||
|
|||
// CreateOptionIpam function returns an option setter for the ipam configuration for this endpoint | |||
func CreateOptionIpam(prefAddress net.IP, ipamOptions map[string]string) EndpointOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of an InterfaceIpamConf
structure so that it is easier to add more fields in future. But I cannot think of any other fields, besides the address and options.
Therefore I think It is better your way, less boilerplate code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that makes sense. I have no problem changing it to that if others feel that it is needed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that only one IP address can be passed. Would it be possible to have multiple per end point ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment. Currently we do not support that, but I think only few changes would make it possible.
Just curious about the use case, is this for IP aliasing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aboch one of the use case is if you want to both have a VIP and a local address. This could be for anycast or implementing direct server return (https://www.nanog.org/meetings/nanog51/presentations/Monday/NANOG51.Talk45.nanog51-Schaumann.pdf). The second IP can be on the loopback or any interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jc-m we do envision the support for properly defined Services and yes, we will have Virtual IPs, Load balancers and all the whole 9 yards. It is just that we are not there yet.
But you did bring up an important point that if you have a custom IPAM driver, then this should be a case of the IPAM driver handling it & libnetwork should not worry about that restriction. But still, even if we make the current PR handle multiple-ip, it stops short at the IPAM layer. The endpoint isnt assigned to multiple IPs when it ends up in the network Driver.
Wont you feel better to have a full e2e support for multiple IPs from the User to IPAM driver and ends up finally in the endpoint as seen by the network driver ?
Again, as @aboch suggested, we are fully supportive of multiple IP assignment for the endpoints for all the use-cases that you suggested. Just that we dont have it covered e2e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mavenugo I obviously care about the e2e support. I see how supporting multiple IPs per endpoints can be a problem with the current model. Another way would have been to pass the preferred IPs in libnetwork.EndpointOptionGeneric , and this would have provided more flexibility. Some drivers could simply ignore them, others could use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jc-m am glad that you brought up these points so that we can get it resolved soon. Will you mind opening an issue/proposal using which we can design it together to support multiple IPs e2e ?
My suggestion would be to let this PR work for a single IP which is supported e2e and work on a proposal to support multiple IPs end-to-end ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with getting this one go with 1 IP for now. We will provide a proposal for the multiple IPs building on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created #798
any updates on getting this merged? |
@rmb938 unfortunately, this has to wait for the next docker release. |
Alright thanks. |
@@ -944,7 +945,10 @@ func (n *network) ipamAllocateVersion(ipVer int, ipam ipamapi.Ipam) error { | |||
// irrespective of whether ipam driver returned a gateway already. | |||
// If none of the above is true, libnetwork will allocate one. | |||
if cfg.Gateway != "" || d.Gateway == nil { | |||
if d.Gateway, _, err = ipam.RequestAddress(d.PoolID, net.ParseIP(cfg.Gateway), nil); err != nil { | |||
var gatewayOpts = map[string]string{ | |||
netlabel.Gateway: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally avoid using an empty string as a way to replace a bool=true
. This is not readable.
Shall we use a more explicit Key and its value can be netlabel.Gateway ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mavenugo I take the blame on this. See #707 (comment)
I am fine to change the value back to "true". This is actually how @rmb938 first wrote it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmb938 i was thinking more like adding a new constant/label in ipamapi package RequestAddressType
as the key and the value can be netlabel.Gateway
:). Can you please make this change and push it ?
We like to get this in soon before the next Vendor into docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mavenugo Go is not my native language so would this be the correct way to do it?
ipamapi/contract.go
In the const declaration add
// RequestAddressType represents the Address Type used when requesting an address
RequestAddressType = "RequestAddressType"
and in network.go
var gatewayOpts = map[string]string{
ipamapi.RequestAddressType: netlabel.Gateway,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmb938 yes. that works.
LGTM. cc @aboch |
@rmb938 Something weird with the commits. Please check. |
fafec47
to
3dcf051
Compare
@aboch how is that? |
3dcf051
to
64e7616
Compare
options. When requesting a gateway address send a gateway label in the options. Signed-off-by: Ryan Belgrave <rmb1993@gmail.com>
b57a937
to
35f1a1f
Compare
@rmb938 Thanks for fixing the multiple commit issue. |
Adds the ability to give driver options when assigning an address.
Also when assigning the ipam gateway address pass the options bellow
This will allow ipam drivers to detect whether an address belongs to a gateway or not.
Currently under CreateEndpoint options are set to nil. This should be changed once EndpointOptions contains driver options for the endpoint it self and driver options for its address.
Signed-off-by: Ryan Belgrave rmb1993@gmail.com